[turbo-tasks]: Manually devirtualize our internal calling conventions#93910
[turbo-tasks]: Manually devirtualize our internal calling conventions#93910lukesandberg wants to merge 19 commits into
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Failing CI jobsCommit: ee7f1dc | About building and testing Next.js |
There was a problem hiding this comment.
Additional Suggestions:
- Type mismatch in
eviction.rscauses undefined behavior: raw pointer cast toProdHandleConcrete(parameterized onProdBackingStorage = Either<TurboBackingStorage, NoopBackingStorage>) but the actualArcholdsTurboTasks<TurboTasksBackend<TurboBackingStorage>>— a different monomorphization.
- Missing
static_handlefeature onturbo-tasks-backenddev-dependency causes test panics withunreachable!()infrom_static_raw().
2edc638 to
f02ec68
Compare
Stats from current PR✅ No significant changes detected📊 All Metrics📖 Metrics GlossaryDev Server Metrics:
Build Metrics:
Change Thresholds:
⚡ Dev Server
📦 Dev Server (Webpack) (Legacy)📦 Dev Server (Webpack)
⚡ Production Builds
📦 Production Builds (Webpack) (Legacy)📦 Production Builds (Webpack)
📦 Bundle SizesBundle Sizes⚡ TurbopackClient Main Bundles
Server Middleware
Build DetailsBuild Manifests
📦 WebpackClient Main Bundles
Polyfills
Pages
Server Edge SSR
Middleware
Build DetailsBuild Manifests
Build Cache
🔄 Shared (bundler-independent)Runtimes
📝 Changed Files (17 files)Files with changes:
View diffsapp-page-exp..ntime.dev.jsfailed to diffapp-page-exp..time.prod.jsDiff too large to display app-page-tur..ntime.dev.jsfailed to diffapp-page-tur..time.prod.jsDiff too large to display app-page-tur..ntime.dev.jsfailed to diffapp-page-tur..time.prod.jsDiff too large to display app-page.runtime.dev.jsfailed to diffapp-page.runtime.prod.jsDiff too large to display pages-api-tu..ntime.dev.jsDiff too large to display pages-api-tu..time.prod.jsDiff too large to display pages-turbo...ntime.dev.jsDiff too large to display pages-turbo...time.prod.jsDiff too large to display server.runtime.prod.jsDiff too large to display use-cache-pr..ntime.dev.jsDiff too large to display use-cache-pr..ntime.dev.jsDiff too large to display use-cache-pr..ntime.dev.jsDiff too large to display use-cache-pr..ntime.dev.jsDiff too large to display 📎 Tarball URLCommit: ee7f1dc |
7fffcd9 to
02d4b42
Compare
81ddf9c to
f84c52e
Compare
This crate is the future home of the #[no_mangle] pub extern "Rust"
fn providers that implement the dispatch surface declared (later in
this phase) in turbo-tasks/src/handle.rs.
For now it is a skeleton: lib.rs is empty (just a doc comment) and
the crate exposes two optional features:
- prod : pulls in turbo-tasks-backend (will emit __tt_prod_*
providers).
- test : pulls in turbo-tasks-testing (will emit __tt_test_*
providers).
Building with neither feature compiles to an empty lib, so 'cargo
check --workspace' works without flags. Binaries and tests that
actually consume the dispatch must enable the appropriate feature(s).
Adds the tagged-pointer dispatch infrastructure that replaces the
Arc<dyn TurboTasksApi> task-local. Only one method (`invalidate`) is on
the dispatch surface so far — subsequent commits will add the rest.
In turbo-tasks/src/handle.rs:
* TurboTasksHandle { tag: HandleTag, ptr: NonNull<()> } — the tagged
pointer that will replace Arc<dyn TurboTasksApi> in TURBO_TASKS.
* unsafe extern "Rust" forward declarations for __tt_prod_invalidate
/ __tt_test_invalidate.
* impl TurboTasksHandle { fn invalidate(...) { match self.tag { ... } } }
— direct dispatch over the tag; both arms are extern "Rust" calls
that thin LTO will inline cross-crate.
* Clone / Drop dispatch through __tt_<arm>_clone_arc / drop_arc so the
underlying Arc refcount stays correct.
* Uses macro_metavar_expr_concat (nightly, stable on our toolchain) to
auto-generate the per-arm symbol names from a single method list.
In turbo-tasks-handle/src/lib.rs:
* ProdHandleConcrete type alias matches what next-napi-bindings uses.
* #[no_mangle] pub extern "Rust" fn __tt_prod_invalidate + clone/drop
providers for the prod arm; mirror for the test arm (VcStorage).
* from_prod / from_test constructors that convert an Arc<...> into
a TurboTasksHandle by Arc::into_raw.
No call sites are switched yet; TURBO_TASKS still holds Arc<dyn
TurboTasksApi>. CI green for all feature combinations of
turbo-tasks-handle (none, prod, test, prod+test).
…d list
Adds the remaining ~21 methods of the TurboTasksApi + TurboTasksCallApi
trait surface to the tagged-pointer dispatch:
TurboTasksCallApi:
dynamic_call, native_call, trait_call, send_compilation_event,
get_task_name
TurboTasksApi:
invalidate (already wired), invalidate_with_reason,
invalidate_serialization, try_read_task_output, try_read_task_cell,
try_read_local_output, read_task_collectibles, emit_collectible,
unemit_collectible, unemit_collectibles, try_read_own_task_cell,
read_own_task_cell, update_own_task_cell, mark_own_task_as_finished,
connect_task, spawn_detached_for_testing,
subscribe_to_compilation_events, is_tracking_dependencies
The following remain OFF the dispatch surface, per the plan:
run, run_once, run_once_with_reason, start_once_process,
stop_and_wait, task_statistics
Every caller of these already has a concrete TurboTasks<B> in scope
(via direct construction in #[tokio::test] / benches / fuzz / the napi
top-level run), so they don't need extern "Rust" dispatch.
Also switches the provider macro from UFCS `<Concrete as
TurboTasksApi>::` to method-call syntax `tt.`, which
resolves both supertrait (TurboTasksCallApi) and subtrait
(TurboTasksApi) methods uniformly.
Cleans up Arc clone/drop to use Arc::increment_strong_count /
decrement_strong_count instead of the from_raw -> clone -> forget
dance. Cleaner and avoids the sketchy debug_assert on pointer
identity.
All four feature combinations of turbo-tasks-handle compile:
none, prod, test, prod+test. Workspace check passes.
The thread-local will soon hold a TurboTasksHandle instead of an
Arc<dyn TurboTasksApi>, so turbo_tasks_weak() needs a parallel
TurboTasksWeakHandle type — the existing Arc::downgrade(arc) path
breaks once the task-local stops holding an Arc.
Adds:
* pub struct TurboTasksWeakHandle { tag, ptr } — mirrors
TurboTasksHandle but holds a Weak<concrete>::into_raw() pointer.
* TurboTasksHandle::downgrade(&self) -> TurboTasksWeakHandle.
* TurboTasksWeakHandle::upgrade(&self) -> Option<TurboTasksHandle>.
* Per-arm extern "Rust" symbols: __tt_<arm>_downgrade, _upgrade,
_clone_weak, _drop_weak. Providers in turbo-tasks-handle round-
trip through Weak::from_raw/Weak::into_raw and Arc::downgrade /
Weak::upgrade.
The plan suggested deferring weak-handle work to a later phase if
benches showed scope-spawning didn't matter. After auditing the
weak-handle consumers (turbo-tasks-fs's filesystem watcher, four
sites — all real and load-bearing), it's cleaner to build the weak
handle now alongside the strong handle than to leave a half-Weak<dyn>
hybrid behind.
No call sites switched yet.
…TurboTasksHandle (WIP)
This is the main flip. Workspace 'cargo check' is green; building test
binaries hits a linkage issue (see end of message).
Core change:
* task_local! { TURBO_TASKS: TurboTasksHandle } in manager.rs (was
Arc<dyn TurboTasksApi>).
* Public accessors turbo_tasks(), try_turbo_tasks(), with_turbo_tasks,
turbo_tasks_weak(), turbo_tasks_scope, turbo_tasks_future_scope,
with_turbo_tasks_for_testing, and the free run/run_once/
run_once_with_reason helpers now return/take TurboTasksHandle.
* TurboTasksWeakHandle parallels TurboTasksHandle for the rare weak
case (used by the fs watcher).
Method dispatch surface expanded:
* run, run_once, run_once_with_reason, start_once_process,
stop_and_wait, task_statistics are now also on the dispatch surface
— the test harness in turbo-tasks-testing type-erases its
TestInstance.tt and calls turbo_tasks::run_once() on it, so 'off
the dispatch surface' didn't hold for these methods after all.
* task_statistics returns &TaskStatisticsApi; the extern provider
returns *const TaskStatisticsApi and the handle wrapper re-binds
the lifetime to &self.
* provide_prod_trait! / provide_test_trait! variants force UFCS for
methods whose inherent signature on TurboTasks<B> shadows the
trait method's signature (the run/stop family).
Internal API signature changes:
* Invalidator::invalidate, invalidate_with_reason now take
&TurboTasksHandle (was &dyn TurboTasksApi).
* read_task_output, read_local_output internal helpers take
&TurboTasksHandle. wait_task_completion's own loop is inlined.
* TurboTaskContextError.turbo_tasks field changed from
Arc<dyn TurboTasksCallApi> to TurboTasksHandle.
* Backend operation::Context::turbo_tasks() returns TurboTasksHandle
(uses thread-local).
Construction sites:
* TurboTasks::make_handle(self: Arc<Self>) -> TurboTasksHandle
inherent method on TurboTasks<B> and on VcStorage.
* make_handle_from_ref(&self) for ergonomic use inside TurboTasks's
own methods that need to scope into themselves.
Downstream updates:
* turbo-tasks-fs DiskFileSystemInner.turbo_tasks Weak<dyn...> →
TurboTasksWeakHandle.
* turbo-tasks-fs watcher signatures changed.
* turbopack-dev-server, turbopack-cli, turbopack-cli's serve()
signature take TurboTasksHandle; UpdateServer::run too.
* turbo-tasks-testing TestInstance.tt and helpers use handles.
Cargo deps:
* next-napi-bindings dev-deps turbo-tasks-handle [features = 'prod'].
* turbo-tasks-backend dev-deps turbo-tasks-handle [features =
'prod', 'test'] for tests + benches.
** KNOWN ISSUE — TEST BUILD LINKAGE **
Test binaries fail to link because cargo doesn't pull
'libturbo_tasks_handle.rlib' into the test binary's link command even
though it's declared as a dev-dep. Investigation needed:
- Likely cause: cargo skips a dev-dep that's never referenced by name
in the test sources.
- Quick workaround: add 'extern crate turbo_tasks_handle;' to each
test file that lives in turbo-tasks-backend/tests/. ~30 files.
- Better solution: probably move providers to a separate
turbo-tasks-prod-handle / turbo-tasks-test-handle pair of crates
that consumers reference explicitly, or use a build-script trick
to force linkage.
The cdylib (next-napi-bindings) does link correctly because cdylib
linking includes all declared deps.
…sting The turbo-tasks-handle crate added in phase 1 created an awkward cycle: it needed turbo-tasks-backend (for ProdHandleConcrete) and turbo-tasks-testing (for VcStorage) as feature-gated deps, which made adding it as a dep of turbo-tasks-backend or turbo-tasks-testing impossible (cycle). The result was per-binary opt-in via dev-deps, which doesn't compose: every test/bench/example crate would need to explicitly enable features on a separate handle crate. This commit collapses turbo-tasks-handle out of existence: * The __tt_prod_* providers move into turbo-tasks-backend/src/ handle_providers.rs. turbo-tasks-backend already owns TurboTasksBackend and the prod handle concrete type. * The __tt_test_* providers move into turbo-tasks-testing/src/ handle_providers.rs. turbo-tasks-testing already owns VcStorage. * The dispatch contract (TurboTasksHandle, HandleTag, forward decls, forwarder macros) stays in turbo-tasks/src/handle.rs. * Two new Cargo features on turbo-tasks: prod_handle, test_handle. Each gates its arm's HandleTag variant, extern decls, and match arms. Single-variant builds (only prod or only test) collapse to a direct call; both-variant builds get cmp + b.ne + direct call. * turbo-tasks-backend deps on turbo-tasks [prod_handle]. turbo-tasks-testing deps on turbo-tasks [test_handle]. * TurboTasks::make_handle is gated behind feature = 'prod_handle'; VcStorage::make_handle is in turbo-tasks-testing so unconditional. Test binaries linking the providers still has a wrinkle: when a crate's test binary unifies test_handle on via dev-deps but the test code doesn't 'use turbo_tasks_testing', cargo doesn't pull libturbo_tasks_testing.rlib into the link command. The fix is 'extern crate turbo_tasks_testing;' in the affected files. So far applied to: - turbo-tasks-backend/src/lib.rs (#[cfg(test)]) - turbo-tasks-backend/tests/eviction.rs - turbo-tasks-backend/benches/mod.rs Still pending: a handful of other workspace crates that build tests without using turbo_tasks_testing directly. Diagnosed but not yet fixed (turbopack-cli, turbopack-tracing, etc.).
The retry/retry_async functions are generic Future utilities with no dependency on turbo-tasks. They previously lived in turbo-tasks-testing where they pulled the rest of that crate (and its test_handle feature activation) into anything that transitively needed retry. Specifically, turbopack-cli dev-depped turbopack-bench which depped turbo-tasks-testing solely for retry(). That dependency triggered turbo-tasks feature unification that enabled test_handle, generating extern decls for __tt_test_* in turbopack-cli's lib test binary, which then failed to link because cargo doesn't pull the unused turbo-tasks-testing rlib into the binary's link command. Moving the two retry helpers to turbopack-bench (their sole user) breaks that chain. turbopack-cli's lib tests now build cleanly.
When 'turbo-tasks' is built standalone with neither 'prod_handle' nor 'test_handle' active, 'HandleTag' previously became a zero-variant enum which is invalid under '#[repr(u8)]' — and downstream consumers (e.g. 'turbo-tasks-bytes') that only declare value types would fail to compile because TurboTasks::make_handle requires 'HandleTag::Prod'. This commit: - Adds 'HandleTag::Unreachable = 255', present only when neither feature is active. The variant is never constructed by consumer code; it just keeps the enum inhabited. - Adds '_ => unreachable!()' arms to every dispatch 'match' (TurboTasksHandle's forwarder macro, task_statistics, Clone, Drop, downgrade, and the same trio for TurboTasksWeakHandle), feature-gated to only exist when no real arm exists. - Ungates TurboTasks::make_handle / make_handle_from_ref so they always exist; in the no-features build they construct a handle with the Unreachable tag (any dispatch through it panics at runtime, but the binary builds). - Adds 'compile_error!' if a build activates 'test_handle' but not 'prod_handle' AND tries to call TurboTasks::make_handle — that combination is structurally inconsistent. The workspace 'cargo check' is green. 'cargo build --workspace --tests --benches' still has linker errors in crates that don't directly dep on turbo-tasks-backend or -testing but get their features unified on by other workspace members. Fixing those needs 'extern crate' anchors in the affected crates (next commit).
Renames the dispatch arms by mechanism (Static/Dynamic) rather than by intended user (Prod/Test). VcStorage from turbo-tasks-testing uses the Dynamic arm because we don't want to wire its concrete type into the static dispatch surface, not because dynamic dispatch is inherently a "test" concept. Also restructures features so static_handle is explicit opt-in (activated by turbo-tasks-backend's static_handle feature, which the napi binding enables) rather than being feature-unified workspace-wide. This avoids the linker error where test binaries that don't directly link turbo-tasks-backend would otherwise see unresolved __tt_static_* extern symbols. Drops the unused read_task_output free function.
The `__tt_static_*` providers in turbo-tasks-backend cast the opaque `*const ()` receiver to `&TurboTasks<TurboTasksBackend<Either<TurboBackingStorage, NoopBackingStorage>>>`. Tests and benches were constructing `TurboTasks::new(TurboTasksBackend::new(_, TurboBackingStorage))` (no Either wrapper), so the resulting `Arc<TurboTasks<_>>` had a different concrete type than what the providers expected — reinterpreting the pointer was undefined behavior and segfaulted under optimization. Adds `ProdBackingStorage` (the Either-wrapped type) and small `prod_backing_storage_turbo` / `prod_backing_storage_noop` helpers in turbo-tasks-backend, then updates every test_config.trs and the three bench harnesses to use them. Also gates the backend's own tests/benches to activate the `static_handle` feature on themselves via a dev-dep self-reference, so `TurboTasksHandle::from_static_raw` is wired up to the actual extern providers instead of the no-feature unreachable! stub.
…nd self-dev-dep `turbo_tasks()` returns a `TurboTasksHandle` by value, not a smart pointer that can be deref'd. The new caller in client.rs needs `&turbo_tasks()` not `&*turbo_tasks()` — bind to a local first so the temporary's drop order is explicit. Also reverts the self-dev-dep on turbo-tasks-backend that activated `static_handle` for its own tests/benches. That activation propagated across the workspace via Cargo feature unification and broke test bins that don't directly link `turbo-tasks-backend` (unresolved `__tt_static_*` extern symbols). Backend tests now panic at runtime without `static_handle`; will reactivate via per-crate dev-deps in a follow-up so only test bins that actually link the backend turn it on.
Resolves the bench-job panic without forcing `static_handle` on workspace-wide (which would spread `__tt_static_*` extern decls into test bins that don't link `turbo-tasks-backend`). When `static_handle` is on (napi binding), `make_handle` returns the Static handle that dispatches through `extern "Rust"` symbols — devirtualized under thin LTO, as before. When `static_handle` is off but `dynamic_handle` is on (workspace tests/benches, which transitively pull in `turbo-tasks-testing`), `make_handle` upcasts the `Arc<TurboTasks<B>>` to `Arc<dyn TurboTasksApi>` and returns a Dynamic handle. One vtable indirection per dispatched call — slower than static, but correct, and only the test/bench path pays it. The third arm (neither feature on) is a runtime panic stub so the generic `impl<B> TurboTasks<B>` block still compiles for crates that only depend on `turbo-tasks` for types.
Removes both the `static_handle` (on `turbo-tasks`) and `dynamic_handle`
(now dead since VcStorage was deleted) Cargo features.
`TurboTasksHandle` is now a single-variant `NonNull<()>`-wrapping type
that always dispatches through `extern "Rust"` symbols defined in
`turbo-tasks-backend`.
This means every binary that links `libturbo_tasks.rlib` must also link
`libturbo_tasks_backend.rlib`. Most workspace crates that depend on
`turbo-tasks` for runtime use already do. For type-declaring leaf
crates (`turbo-tasks-bytes`, `turbo-esregex`, `turbopack-css`, etc.)
whose lib-test binaries link the rlib but otherwise have no need for
the backend, the convention is a `[dev-dependencies] turbo-tasks-backend
= { workspace = true }` plus `#[cfg(test)] extern crate
turbo_tasks_backend;` in `src/lib.rs` — cargo won't include the rlib in
the link command unless Rust code references it.
A follow-up could replace the unconditional extern decls with weak
symbols (`#[linkage = "extern_weak"]`), letting the dispatch site
resolve to a null function pointer for crates that never actually
construct a `TurboTasks<B>`. Under thin LTO the resulting null-check
should constant-fold away in callers that do link the backend. Not
done here because it would require nightly-only `linkage` attribute
support and `Option<unsafe extern "Rust" fn(...)>` plumbing.
Each dispatched method previously required two macro invocations that duplicated the signature: tt_decl_extern! for the extern declaration and tt_decl_handle_method! for the matching inherent method on TurboTasksHandle. Merge them into a single tt_decl! that emits both.
The only caller (next-napi-bindings/src/next_api/project.rs:2142) holds the concrete `Arc<TurboTasks<TurboTasksBackend<TurboBackingStorage>>>` and goes through the trait method via static dispatch. No call site reaches it via `TurboTasksHandle`, so the extern + handle wrapper were dead weight.
Both APIs previously took a `TurboTasksHandle`, forcing callers like
turbopack-cli to erase their concrete `Arc<TurboTasks<Backend>>` through
the extern-dispatch handle. Make them generic over `T: TurboTasksApi`
and `T: TurboTasksCallApi` respectively, so the concrete backend type
flows through and trait methods monomorphize directly.
The two `run_once_with_reason` free-helper calls in dev-server are
inlined: the request path uses a oneshot channel to ferry the response
out of the `Result<()>` trait method, and the side-effects path spawns
its closure inside `tokio::spawn(async move { tt.run_once_with_reason(...) })`
so the boxed future owns its `Arc<T>` clone (the trait method's future
isn't `'static`).
…andle These four methods were only routed through the type-erased TurboTasksHandle so test code could hold them. Every production caller already has the concrete Arc<TurboTasks<TurboTasksBackend<_>>> and uses the inherent methods directly. Make TestInstance carry the concrete arc (Arc<TurboTasks<TurboTasksBackend<TurboBackingStorage>>>) instead of a TurboTasksHandle — all .trs files produce that exact type, so Registration / TestInstance stay non-generic. Migrate the test-only turbo_tasks::run / run_once / run_once_with_reason free-fn shims into turbo-tasks-testing, where they can call the inherent methods (which already return Result<T> directly, so the oneshot-channel wrapper is unnecessary). Then delete: the four tt_decl entries, their providers in handle_providers.rs, the now-unused provide_prod_trait! macro, the three free-fn shims in manager.rs, and the matching trait methods on TurboTasksCallApi (run, run_once) and TurboTasksApi (stop_and_wait). TurboTasksCallApi::run_once_with_reason stays — the dev-server uses it through Arc<dyn TurboTasksApi>.
d4dde43 to
ee7f1dc
Compare

No description provided.